Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: Adding PR Build and Deploy actions for all PRs #1056

Merged
merged 1 commit into from
Nov 13, 2024
Merged

feat: Adding PR Build and Deploy actions for all PRs #1056

merged 1 commit into from
Nov 13, 2024

Conversation

pdellaert
Copy link
Contributor

Summary

Uses https://github.com/marketplace/actions/refined-cloudflare-pages-action#enabling-pr-previews-from-forks for building and deploying PR Previews to Cloudflare pages.

Setup

Requires a few new secrets:

  • CLOUDFLARE_PAGES_API_TOKEN - Specific API Token with access to editing Cloudflare Pages for the account
  • CLOUDFLARE_PAGES_ACCOUNT_ID - API Account ID for Cloudflare Pages, this could be the same as the main account, but might be good to split up in case we ever need to have flexibility.
  • CLOUDFLARE_DOCS_PROJECT - The Pages project in Cloudflare for the docs site specifically.

On Cloudflare Pages, it is best to only allow automatic building of the main branch (primary) of the main repo, and not other branches, although this is not a must. If building all branches remains enabled, if a PR is created for a branch on the main repo, two deployments will exist for the PR. One for the branch, and one for the PR itself (using this change).

Functionality

This is a two-stepped approach so that PRs do not need access to secrets (fork PRs have no access to secrets of the main repo):

  1. PR Build & Upload - This builds and uploads an archive of the build locally for the PR itself. This is triggered by opening or syncing a PR (pushing to it)
  2. PR Deploy - This runs after the PR Build, and runs inside the secure context of the main repo, with no visibility into the secrets by the PR or PR Repo. It takes the archive and creates a Preview deployment for the PR (name is <repo-owner>-<branch>.<project>.pages.dev.
  3. The action creates or updates (if new commits) a comment on the PR with the Preview deployment link after build

Proof of concept

Both have been deployed on CF Pages as previews, with multiple commits (create PR, and adding more commits after):
image
Note that in this test deployment, only Primary (production) branch is enabled for automatic build. For other branches on the main repo, the automatic builds are disabled.

@pdellaert pdellaert added the do not merge PRs not to be merged before this label has been removed label Nov 11, 2024
@pdellaert pdellaert self-assigned this Nov 11, 2024
@github-actions github-actions bot added the Review Required PR Check Label label Nov 11, 2024
@github-actions GitHub Actions
Copy link

This PR is being prevented from merging because Review Required. Use the Approved label to run build validation and auto merge the PR.

@Valastiri
Copy link
Member

Valastiri commented Nov 11, 2024 •
edited
Loading

Noted will review. Small point in regards to the following:

On Cloudflare Pages, it is best to only allow automatic building of the main branch (primary) of the main repo, and not other branches, although this is not a must. If building all branches remains enabled, if a PR is created for a branch on the main repo, two deployments will exist for the PR. One for the branch, and one for the PR itself (using this change).

Quick first question is does this change overwrite the current functionality of having a preview build for any feature branch (on main repo) that has a PR to primary? Bit confused apologies.

If no we don't really need to build other branches unless they are PR'd since they are just technically working feature branches. I know that vercel did this and allowed us to technically preview feature branches that did not have a PR but we never really needed to use this feature.

Second question - with the current iteration of this PR if I created a PR as previously for say multi-aircraft-restructure which had the following:

  • multi-aircraft-restructure had a PR -> primary
  • There were multiple working PRs against multi-aircraft-restructure

Would the second bullet point still generate a PR preview?

@pdellaert
Copy link
Contributor Author

Noted will review. Small point in regards to the following:

On Cloudflare Pages, it is best to only allow automatic building of the main branch (primary) of the main repo, and not other branches, although this is not a must. If building all branches remains enabled, if a PR is created for a branch on the main repo, two deployments will exist for the PR. One for the branch, and one for the PR itself (using this change).

Quick first question is does this change overwrite the current functionality of having a preview build for any feature branch (on main repo) that has a PR to primary? Bit confused apologies.

If no we don't really need to build other branches unless they are PR'd since they are just technically working feature branches. I know that vercel did this and allowed us to technically preview feature branches that did not have a PR but we never really needed to use this feature.

@Valastiri
The behavior when disabling automatic builds in CF itself, in combo with this PR, is such:
If a (feature) branch is created on the main repo, no preview is build and deployed until a PR is opened for that branch to be merged in another branch.
Nothing stops us from enabling automatic build for all (feature) branches, if then a PR is opened for the feature branch, it would be deployed twice: Once by CF automatically for the branch; Once for the PR by the new workflows.

The initial suggestion from the custom action we are using here, is to completely disable Cloudflare Github direct integration. Which means no CF Automatic builds on Production (Primary) branch. If we do that, we would also need to create a similar workflow for changes on the primary branch (and optionally one for feature branches).

But I find CF building the primary branch automatically is probably a nice thing. And have PRs be managed by these workflows.

Second question - with the current iteration of this PR if I created a PR as previously for say multi-aircraft-restructure which had the following:

  • multi-aircraft-restructure had a PR -> primary
  • There were multiple working PRs against multi-aircraft-restructure

Would the second bullet point still generate a PR preview?

Yep, any PR would generate a preview.

frankkopp
frankkopp approved these changes Nov 11, 2024
View reviewed changes
Copy link
Member

@frankkopp frankkopp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As we have no way to test it other the merge and test it I'd say let's go for it and see if we need to change something later.

@pdellaert pdellaert removed do not merge PRs not to be merged before this label has been removed Review Required PR Check Label labels Nov 11, 2024
@github-actions github-actions bot added the Review Required PR Check Label label Nov 11, 2024
@pdellaert pdellaert added Approved and removed Review Required PR Check Label labels Nov 13, 2024
@github-actions github-actions bot added Review Required PR Check Label and removed Approved labels Nov 13, 2024
@frankkopp frankkopp merged commit caabb30 into flybywiresim:primary Nov 13, 2024
16 of 22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@frankkopp frankkopp frankkopp approved these changes

@Valastiri Valastiri Valastiri approved these changes

Assignees

@pdellaert pdellaert

Labels
Review Required PR Check Label
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@pdellaert @Valastiri @frankkopp